SOLR-18072: Refactor CollectionApiCommands to add expandable context #4046

HoustonPutman · 2026-01-13T20:55:44Z

https://issues.apache.org/jira/browse/SOLR-18072

There are no functionality changes here. Just test improvements, and refactoring of the APIs.

I'm also happy to call AdminCmdContext something else, that's a very easy change.

dsmiley

I overall like the proposal of an Admin API context class for these APIs.

I've been looking in this PR for where SubResponseAccumulatingJerseyResponse is defined but can't find it; maybe because Safari/GitHub is completely choking on this massive PR. I don't love the name of that thing. Can you please self-review to identify it?

gradle/validation/validate-source-patterns.gradle

solr/core/src/java/org/apache/solr/handler/admin/CollectionsHandler.java

solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java

solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java

dsmiley · 2026-01-14T05:50:56Z

solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java

+    CollectionAdminRequest.RequestStatus requestStatus =
+        CollectionAdminRequest.requestStatus(asyncId);
+    CollectionAdminRequest.RequestStatusResponse rsp = null;
+    TimeOut timeoutCheck = new TimeOut(timeout, TimeSource.NANO_TIME);


hmm; we also have org.apache.solr.common.util.RetryUtil utility. Maybe that should at least cross-link.

I can switch over to that.

dsmiley · 2026-01-14T05:53:29Z

solr/core/src/test/org/apache/solr/handler/admin/api/MockAPITest.java

+import org.mockito.ArgumentCaptor;
+
+/**
+ * Abstract test class to setup shared mocks for unit testing v2 API calls that go to the Overseer.


Overseer or DistributedCollectionConfigSetCommandRunner, fixed.

dsmiley · 2026-01-14T05:56:08Z

solr/core/src/test/org/apache/solr/handler/admin/api/MockAPITest.java

Based on the content here, this seems actually for SolrCloud. If so; maybe the name or package should reflect that. Any way, this class is peculiar to me.

I could move it, but all of these test classes already lived here, and this is just an abstract (will change) class to help with the mocking for most of the other test classes in this package.

HoustonPutman · 2026-01-14T16:37:19Z

I've been looking in this PR for where SubResponseAccumulatingJerseyResponse is defined but can't find it; maybe because Safari/GitHub is completely choking on this massive PR. I don't love the name of that thing. Can you please self-review to identify it?

This response class already exists, and the method that populated it already existed too. I just expanded the use of it, so that we weren't doing the same thing across each API independently. That's why you can't find it in the PR. We can change the name independently if you dislike it, but not really in the scope of this PR.

…dmin-cmd-apis

dsmiley

Thanks.

dsmiley · 2026-01-14T22:46:49Z

solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java

-    }
-    fail("Async request " + asyncId + " did not complete within duration: " + timeout.toString());
-    return rsp;
+    final AtomicReference<CollectionAdminRequest.RequestStatusResponse> rsp =


This looks worse. I guess TimeOut is simpler in part due to no try-catch needed in some cases (like this) and no AtomicReference needed either. Dodging the try-catch need is unusual... normally a failing request propagates an exception but I see the API you are using doesn't do that. I think that's a known GAP in our APIs that I had been chatting with Jason about solving.

Any way, do as you please.

Yeah, agreed.

Ok at first I was going to just revert the change. But then I decided adding an additional retry utility that would return a response, that is validated, would be better. The method looks MUCH better now.

gerlowskija

Publishing a partial review to make a few in-line comments visible. Hoping to have the second half of my review available shortly...

gerlowskija · 2026-01-15T16:55:36Z

solr/api/src/java/org/apache/solr/client/api/endpoint/DeleteAliasApi.java

      summary = "Deletes an alias by its name",
      tags = {"aliases"})
-  SolrJerseyResponse deleteAlias(
+  SubResponseAccumulatingJerseyResponse deleteAlias(


[Q] Why are we narrowing the response schema of this API?

Typically "SubResponseAccumulatingJerseyResponse" gets used for those admin APIs where the overseer sends out requests to individual replicas, etc. and includes each of those sub-responses in what gets returned to the original caller. But looking at DeleteAliasCmd, it doesn't seem to be doing anything like that?

There's a (minor?) downside to doing this in that our OpenAPI spec (and everything that gets generated from it) will now look like the additional fields will be present in the API response.

(Note: this question applies to a few of the files above as well, but just leaving it in this one place to avoid duplication.)

gerlowskija · 2026-01-15T17:18:09Z

solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java

          collectionParams,
          ccc.getCoreContainer().getConfigSetService());

+      Map<String, Object> propMap = new HashMap<>();


[0] I've seen a few little snippets like this one so far; might be worth some kind of "cloneZkPropsWithOperation" utility method 🤷

.../java/org/apache/solr/cloud/api/collections/DistributedCollectionConfigSetCommandRunner.java

gerlowskija · 2026-01-15T17:43:52Z

solr/core/src/java/org/apache/solr/cloud/api/collections/MigrateCmd.java

-      String internalAsyncId = asyncId + Math.abs(System.nanoTime());
-      props.put(ASYNC, internalAsyncId);
+    String internalAsyncId = null;
+    if (adminCmdContext.getAsyncId() != null) {


[Q] Wdyt of having some sort of getInternalAsyncId() or getSubrequestAsyncId() method in AdminCmdContext that would encapsulate this logic?

Seems like it's something a number of cmd implementations have to figure out for themselves, and would definitely fit under the general umbrella of "admin command context" conceptually.

gerlowskija

Looks great overall!

I left a few minor comments here and there - I think the most significant of which are around some of the new SubResponseAccumulatingJerseyResponse usages. But this looks great, even with those quibbles.

✅

gerlowskija · 2026-01-15T19:32:46Z

solr/core/src/java/org/apache/solr/handler/admin/api/AddReplicaProperty.java

  @Override
  @PermissionName(COLL_EDIT_PERM)
-  public SolrJerseyResponse addReplicaProperty(
+  public SubResponseAccumulatingJerseyResponse addReplicaProperty(


[+1] Good catch

gerlowskija · 2026-01-15T19:39:27Z

solr/core/src/java/org/apache/solr/handler/admin/api/AliasProperty.java

  @PermissionName(COLL_EDIT_PERM)
-  public SolrJerseyResponse deleteAliasProperty(String aliasName, String propName)
-      throws Exception {
+  public SubResponseAccumulatingJerseyResponse deleteAliasProperty(


[-0] All of the "update" methods in this class shouldn't return SubResponseAccumulatingJerseyResponse, afaict, because the API response never uses the "successes" or "failures" fields that are the main reason for SubResponse... to exist.

If we aim of this change is to support async alias-prop modification, we can probably use AsyncJerseyResponse instead?

gerlowskija · 2026-01-15T19:42:55Z

solr/core/src/java/org/apache/solr/handler/admin/api/CreateAlias.java

-  public SolrJerseyResponse createAlias(CreateAliasRequestBody requestBody) throws Exception {
-    final SubResponseAccumulatingJerseyResponse response =
-        instantiateJerseyResponse(SubResponseAccumulatingJerseyResponse.class);
+  public SubResponseAccumulatingJerseyResponse createAlias(CreateAliasRequestBody requestBody)


[-0] Idk why L88 was instantiating a "SubResponseAccumulatingJerseyResponse" prior to this PR, maybe a copy-paste error on my part?

But in reality, this API never sets the "successes" or "failures" fields that are the main reason for SubResponse... to exist. So rather than switching the return type here to SubResponse, we should probably make it "SolrJerseyResponse" in both places? Or maybe "AsyncJerseyResponse" if we want to support asynchronous alias creation?

gerlowskija · 2026-01-15T19:46:44Z

solr/core/src/java/org/apache/solr/handler/admin/api/CreateCollection.java

-      throw remoteResponse.getException();
-    }
-
-    if (requestBody.async != null) {


[+1] Such a huge improvement that each individual API doesn't need to check for or copy over these fields anymore!

gerlowskija · 2026-01-15T19:48:32Z

solr/core/src/java/org/apache/solr/handler/admin/api/DeleteAlias.java

-      response.requestId = asyncId;
-    }
-
+  public SubResponseAccumulatingJerseyResponse deleteAlias(String aliasName, String asyncId)


[-0] Another place, afaict, where the original "SolrJerseyResponse" better reflects the actual response content that we expect.

gerlowskija · 2026-01-15T20:39:02Z

solr/core/src/test/org/apache/solr/handler/admin/api/DeleteNodeAPITest.java

+    ArrayList<String> l = new ArrayList<>(liveNodes);
+    Collections.shuffle(l, random());
+    CollectionAdminRequest.Create create =
+        pickRandom(


[+1] This is a cool little pattern 👍

gerlowskija · 2026-01-15T20:42:41Z

solr/core/src/test/org/apache/solr/handler/admin/api/MockAPITest.java

+ * Abstract test class to setup shared mocks for unit testing v2 API calls that go to the Overseer
+ * or the DistributedCollectionConfigSetCommandRunner.
+ */
+public abstract class MockAPITest extends SolrTestCaseJ4 {


[Q] Should the test name have something in it to indicate it's "v2"-ness, and that it's not just for any old API you want to mock?

gerlowskija · 2026-01-15T20:46:20Z

solr/core/src/test/org/apache/solr/handler/admin/api/RestoreCollectionAPITest.java

+    assertEquals("someRepository", remoteMessage.get(BACKUP_REPOSITORY));
    assertEquals("someBackupName", remoteMessage.get(NAME));
+
+    AdminCmdContext context = contextCapturer.getValue();


[0] Most (all?) of these mock-based v2 API tests have some version of this little snippet where we're grabbing the AdminCmdContext out of a capturer and asserting the action and async ID. Just flagging it as there might be a way to pull it into a helper method in MockAPITest.

gerlowskija · 2026-01-15T20:53:27Z

solr/solrj/src/java/org/apache/solr/client/solrj/response/RequestStatusState.java

  }

+  /** Returns whether the request status should be refreshed given the current state. */
+  public boolean shouldRefresh() {


[0] Idk if "refresh" means anything to most users. Maybe isOperationFinished or something similar? Or maybe ignore me and "refresh" is the best option as anything else would risk causing confusion with the "COMPLETED" enum value.

Ignore me, just thinking aloud I guess...

I agree on "refresh" not being the most clear word. Maybe isFinal or isFinished, which implies you may want to get the state again.

gerlowskija · 2026-01-15T20:55:28Z

solr/test-framework/src/java/org/apache/solr/cloud/SolrCloudTestCase.java

    }
  }

+  protected CollectionAdminRequest.RequestStatusResponse waitForAsyncClusterRequest(


[0] Is there anything SolrCloudTestCase-specific about this utility? Not all of our SolrCloud tests use SolrCloudTestCase, so it might be nice to put it in some place where it can be used by any SolrCloud-relevant test 🤷

dsmiley · 2026-01-15T23:35:55Z

solr/solrj/src/java/org/apache/solr/client/solrj/response/RequestStatusState.java

  }

+  /** Returns whether the request status should be refreshed given the current state. */
+  public boolean shouldRefresh() {


I agree on "refresh" not being the most clear word. Maybe isFinal or isFinished, which implies you may want to get the state again.

dsmiley · 2026-01-15T23:37:57Z

solr/solrj/src/java/org/apache/solr/common/util/RetryUtil.java

+   *     an exception
+   * @throws SolrException if all retries are exhausted without success
+   */
+  public static <T> T retryUntil(


SOLR-18072: Refactor CollectionApiCommands to add expandable context

920ba91

HoustonPutman requested a review from gerlowskija January 13, 2026 20:55

github-actions bot added test-framework client:solrj tool:build tests cat:cloud cat:api labels Jan 13, 2026

HoustonPutman added 3 commits January 13, 2026 13:09

Errorprone fix

f2e3966

Fix bugs

d9d4af3

Fix new bug

1d73112

dsmiley reviewed Jan 14, 2026

View reviewed changes

HoustonPutman added 3 commits January 14, 2026 09:09

Merge remote-tracking branch 'apache/main' into solr-18072-refactor-a…

cfc219c

…dmin-cmd-apis

Address PR review

a2c39bc

Tidy

320675c

github-actions bot removed the client:solrj label Jan 14, 2026

dsmiley approved these changes Jan 14, 2026

View reviewed changes

HoustonPutman added 2 commits January 14, 2026 15:53

Undo test utility change

9da3ee9

Much cleaner retry utility

0967535

github-actions bot added the client:solrj label Jan 15, 2026

gerlowskija reviewed Jan 15, 2026

View reviewed changes

gerlowskija approved these changes Jan 15, 2026

View reviewed changes

dsmiley reviewed Jan 15, 2026

View reviewed changes

SOLR-18072: Refactor CollectionApiCommands to add expandable context #4046

Are you sure you want to change the base?

SOLR-18072: Refactor CollectionApiCommands to add expandable context #4046

Uh oh!

Conversation

HoustonPutman commented Jan 13, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoustonPutman commented Jan 14, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development